feat: add AnswerSettings and QuestionSettingsHeader components - #6059

Oh, given that this will only be used in the editor, could we move this component to the choice folder instead?

Given that this will only be rendered for choice interactions, I think it's fine to let it infer when to display each based on the questionType instead of this settings prop.

I think we can leave the normal text color here, instead of this annotation.

We can also just use the cancelText prop instead of this actions slot. Usually the actions slot is used for more complex button layouts.

Idem

Actually, the most important responsibility of this component is the type selector, so could we reference "type selector" in the name instead of "SettingsHeader"? (Similarly for class names like question-settings-header, etc)

We sometimes have issues because of rendering conditions on the root element of a component, and a lint rule will soon be added to avoid this, so could we move forward with this condition and leave this responsibility to the parent component instead? i.e. let the parent do <QuestionTypeSelector v-if="mode==='edit'" instead

Oh, some comments: the interaction editors should not be the ones responsible for rendering this "settings header," which is actually a type selector, mainly because this will cause the DOM to remove these nodes when the component is unmounted. The type selector (and therefore, the whole header row) should be shared across all interaction editors and rendered independently of them, so that if we change the question type, the type selector component is not removed from the DOM (which would cause some accessibility issues).

So, what we can do instead is:

• Have the InteractionSection component be the one that renders the QuestionTypeSelector component. This way, each interaction has its own type selector independent of the editor being rendered.
• Let the QuestionTypeSelector be the one responsible for rendering the div with the proper ID so that editor components can target it (instead of using a slot).
• We can take advantage of the fact that we can only have one item being edited at a time, and because of this, there will always only be one question type selector rendered at a time, and use a constant as id so that we don't have to keep track of any identifiers yet.

With this, the idea would be:

// QuestionTypeSelector
<div class="type-selector>
  ...
  <div id="qti-interaction-settings" />
...

// InteractionSection
<QuestionTypeSelector ... />
<component :is="descriptor.editorComponent" ... />

// ChoiceInteractionEditor
<div class="choice-editor">
  <Teleport to="qti-interaction-settings">
    <AnswerSettings ... />
  </Teleport>
  ....
</div>

This way only the interaction editors that actually wants to add settings are the only ones that needs to teleport anything.

Another little problem with this is that the questionType selector should render all question types for all interactions, not only the ones they are related to.

✅ Resolved — addressed in the current code.

blocking: descriptors is the full registry (interactions/index.js:15), so a choice question's KSelect offers all five types, including the text-entry ones. Selecting one is a data-loss path: descriptor recomputes to the text-entry descriptor (line 64), <component :is> mounts TextEntryEditor with :interaction still holding the choice block, and its { immediate: true } watcher on workingInteraction (TextEntryEditor.vue:340) emits update:interaction on mount — which QTIItemEditor.onUpdateInteraction writes straight into currentBodyXml. Both descriptors declare convertsFrom = [], so there is no conversion step; the author's choice question is just replaced.

This is also outside #6033's scope ("The modal lists only the types that the current interaction plugin supports") and fails the criterion "Selector shows 'Single choice' and 'Multiple choice' for choice interactions".

Source the options from the resolved descriptor instead:

const typeOptions = computed(() => descriptor.value.getTypeOptions?.(qtiEditorStrings) ?? []);

That also makes :disabled="questionTypeOptions.length <= 1" meaningful, and lets TextEntryInteractionDescriptor.getTypeOptions stay for when text-entry switching is actually built. Note selectedOption in QuestionTypeSelector would then need a guard — questionTypeOptions[0] is undefined for a descriptor without getTypeOptions, and the template dereferences selectedOption.label.

Yeah, we should not have this coming from useInteractionDescriptor as it does not depend on the descriptor. The TypeSelector component should be able to declare its own list of type options instead of having it come from props.

You're right, and I was wrong on the scope half of this — you've stated the selector should render all question types across all interactions, so the full registry is intentional and my "outside #6033's scope" objection doesn't hold. Withdrawing the descriptor-sourced typeOptions remedy too; the TypeSelector owning its own list is your call and I'll defer to it. Apologies for the noise.

✅ Resolved — addressed in the current code.

blocking: the list only contains KRadioButtons when isSingleSelect is true — multi-select renders KCheckbox (lines ~120-140). KDS KRadioButtonGroup.vue mounted() (verified in node_modules, lines 41-56) does:

this.lastRadioIdx = this.radioButtons.length - 1;
const firstRadioButton = this.radioButtons[this.focusedRadioIdx];  // undefined
firstRadioButton.setTabIndex(0);                                   // TypeError

with no empty guard. The whole block is behind if (!this.isFirefox) return;, so this won't show up in Jest or Chrome QA, but it breaks the editor for every multiple-choice question in Firefox. Same crash for a single-choice question where every choice has a validation error, since the error KIcon replaces the radio button.

Separately, KRadioButtonGroup hard-codes role="radiogroup", so wrapping checkboxes misreports the control type and applies roving arrow-key navigation where Tab-per-checkbox is correct.

channelEdit/components/AnswersEditor/AnswersEditor.vue:11 already has the pattern:

<component :is="shouldHaveOneCorrectAnswer ? 'KRadioButtonGroup' : 'div'">

This change isn't called for by #6033 at all, so reverting it is equally valid. If the wrapper stays for single-select, give it aria-labelledby pointing at the "Answers" heading.

✅ Resolved — addressed in the current code.

blocking: there is no questionType branch here, so the multi-select rule is applied to single choice as well, and buildChoiceInteractionXML writes state.maxChoices verbatim (interactions/choice/parse.js:126). Three concrete regressions:

• Single choice with no correct answer marked yet → max-choices="0" (unlimited) instead of 1.
• Switching multiple → single with 3 answers marked correct → max-choices="3" alongside cardinality="single"; toggleCorrectChoice doesn't prune on the type switch.
• Every single-choice question with a correct answer now also emits min-choices="1" (previously dropped, since parsed minChoices was 0), silently making it mandatory to answer.

#6033 is explicit: "For single choice this checkbox is hidden — max-choices is always 1." Gating stateForXml on questionType.value === QuestionType.SINGLE_SELECT, and leaving minChoices at the parsed value in that case, covers all three.

✅ Resolved — addressed in the current code.

suggestion: this global registration is the only one — jest_config/setup.js registers only ActionLink, so in tests <Teleport> renders as an unknown element with its children in place, and Vue.config.silent = true suppresses the warning. That means the whole describe('Answer settings') block in ChoiceInteractionEditor.spec.js passes because the label rendered inline, not at the teleport target; target resolution, the settingsTargetId string surgery, and the ordering of the two teleported blocks are all untested.

It also makes these components silently depend on shared/app.js having run, which is a bad fit for code living in shared/views/. Importing Teleport from 'vue2-teleport' locally in InteractionSection and ChoiceInteractionEditor (and dropping the global registration) fixes both, and avoids squatting on a Vue 3 built-in name. Worth one integration assertion in QTIItemEditor.spec.js that the settings land inside #qti-question-settings-0.

Yeah, let's just import vue2-teleport directly where we need it.

✅ Resolved — addressed in the current code.

suggestion: #qti-question-settings exists nowhere in the app — only the -${index} variant does. Same for InteractionSection's 'qti-interaction-settings' fallback (index.vue:75). They also interact badly: if InteractionSection renders without teleportTarget, v-if="teleportTarget" skips QuestionTypeSelector, so the #qti-interaction-settings div is never created — yet line 36 still passes that truthy string down, so AnswerSettings teleports into a querySelector that resolves to null and the settings silently vanish.

Make both ids required: true with no default, and pass one id consistently instead of stripping and re-adding the # across three components. Better still: if the descriptor exposed a settingsComponent alongside editorComponent, InteractionSection could render it into a QuestionTypeSelector slot directly — no ids, no teleport, no test-environment divergence.

✅ Resolved — addressed in the current code.

suggestion: neither assertion touches :disabled — the hidden input exists with two options too, and the "Type" label is static. This test passes with :disabled deleted entirely. Assert on the control: expect(screen.getByRole('combobox')).toBeDisabled().

More importantly, the component's one behaviour — @update:questionType (index.vue:23) — has no test, and neither do the two acceptance criteria with real logic behind them: "changing the type updates questionType and re-renders ChoiceInteractionEditor", and "cardinality in the response declaration XML updates to match the new type". The second is the one that has to survive any refactor of the buildXML plumbing; a useChoiceInteraction spec that flips questionTypeRef and parses responseDeclarations.value[0] covers it cheaply.

✅ Resolved — addressed in the current code.

nitpick: a two-statement handler that writes to a setup-owned ref from a template expression. It works in Vue 2.7 (proxyWithRefUnwrap forwards the set), but it hides the mutation from the script block — and the watch(questionType, ..., { immediate: true }) at line ~55 already emits update:questionType, so each selection emits twice. A named onQuestionTypeChange(newType) in setup that only assigns the ref, letting the watcher emit, is clearer and emits once.

Related: currentQuestionType is stored in QTIItemEditor but never fed back as a prop, so this ref is the real source of truth. Fine functionally — worth a comment so the duplicated state isn't read as a wiring bug later.

Yeah, it'd be best to have named functions coming from the setup method.

✅ Resolved — addressed in the current code.

nitpick: unique across questions in one QTIEditor, but vue2-teleport resolves targets with document.querySelector, which takes the first document-wide match. Two QTIEditor instances on a page would teleport both selectors into the first card. Suffixing with _uid makes it collision-proof for one interpolation.

✅ Resolved — addressed in the current code.

praise: the conventions that usually slip are all clean here — no @media (useKResponsiveWindow drives .small-screen), no hard-coded colours, and the only inline bindings are color and borderBottom, both direction-agnostic so RTLCSS has nothing to flip.

I don't think we need this teleport here, right? We only need to teleport from the editor to the interaction section, not from the interaction section to the QTI item editor, or is this something needed? The idea is that we should have one type selector per interaction; this is why it should go next to the interaction and not in the general qti-item-editor component, because in the future we can have one item with multiple interactions.

We are having this bug, very likely because of this teleport, when we reorder the questions, the settings node keeps staying on the same item:

Yeah, it'd be best to have named functions coming from the setup method.

I don't think we need this after teleportTarget is removed, right?

We'll need to be careful if we use index as IDs here, because this is reorderable and that could lead to unexpected behavior; this is similar to https://vuejs.org/guide/essentials/list.html#maintaining-state-with-key.

Idem, lets use the modal props instead.

Yeah, we should not have this coming from useInteractionDescriptor as it does not depend on the descriptor. The TypeSelector component should be able to declare its own list of type options instead of having it come from props.

Yeah, let's just import vue2-teleport directly where we need it.

I'm not sure how the initial state is preserved here 😅, but this should not happen as it may be confusing to users. If we ever change from choice interaction -> text entry interaction, the choice interaction state should be lost, and if we change back from text entry interaction -> choice interaction, then the choice interaction should be empty, not populated with the previous state.

✅ Resolved — addressed in the current code.

blocking: this flat-maps over the whole registry — descriptors is [choiceDescriptor, textEntryDescriptor] (interactions/index.js:15) and TextEntryInteractionDescriptor.getTypeOptions returns Numeric / Text entry / Free response — so a choice question's dropdown offers five types.

Selecting Numeric on a finished multiple-choice question runs InteractionSection.onUpdateQuestionType (components/InteractionSection/index.vue:61-78), which builds freshState from newDescriptor.parse('', []) and carries across only prompt. Every choice, its content, and the correct-answer declarations are dropped and update:interaction is emitted with the replacement XML — no confirmation, no undo. Switching back yields a single empty choice.

#6033 scopes this out explicitly: "The modal lists only the types that the current interaction plugin supports (for choice: Single choice and Multiple choice)", and out of scope: "Any question type other than single choice and multiple choice."

Source the options from the resolved descriptor — which is what putting getTypeOptions on the descriptor was for. InteractionSection already holds it:

// InteractionSection
const questionTypeOptions = computed(() => descriptor.value.getTypeOptions?.(qtiEditorStrings) ?? []);

and pass it in as a prop. That drops the cross-descriptor conversion in onUpdateQuestionType entirely (choice↔choice needs none), makes :disabled="questionTypeOptions.length <= 1" meaningful, and leaves TextEntryInteractionDescriptor.getTypeOptions in place for when text-entry switching is actually built. Keep the existing || { label: '', value: '' } guard on selectedOption, since a descriptor without getTypeOptions yields an empty list.

Two symptoms of the same coupling show up in the tests: QuestionTypeSelector.spec.js has to jest.mock('../../../interactions') to control the options at all, and it('disables selector when only one option available') asserts a state that cannot occur in production.

✅ Resolved — addressed in the current code.

suggestion: shouldRenderRadioGroup reads choiceHasError, which reads errors — written by the debounced validator in useInteraction.js (400 ms after the author stops typing). When is changes the tag the vnodes aren't sameVnode, so Vue replaces the element and rebuilds the whole subtree, including every TipTapEditor in the list.

On a new single-choice question (_defaultState() gives one empty choice): t=0 errors empty → KRadioButtonGroup; t=400 ms the empty-choice error lands → tag swaps to div, list remounts; author types and pauses → error clears → swaps back, remounting again and taking focus and cursor position out of the open editor mid-edit.

The some(...) guard is presumably there because a fully-errored list renders KIcon instead of KRadioButton (lines 120-126), leaving the group with no radio children. Keeping the container tag stable avoids the remount either way — either always use KRadioButtonGroup for single-select and let it handle an empty radio set, or always render a plain div with role="radiogroup" and drop the dynamic component.

suggestion: the teleport bridge — the whole point of the PR — has no test. AnswerSettings reaching the selector's target is the one behaviour that can silently fail in production (target not in the document when vue2-teleport resolves #${teleportTargetId} on mount → settings simply don't render).

Nothing covers it today: ChoiceInteractionEditor.spec.js creates its own #test-settings-target on document.body, QuestionTypeSelector.spec.js renders the target but no teleport source, and InteractionSection.spec.js renders both halves but asserts nothing about answer settings. This component owns settingsTargetId and renders both halves, so it's the natural home:

it('renders answer settings inside the question type selector', () => {
  renderSection({ interaction: interactionBlock(CHOICE_MULTI_SELECT_XML) });
  expect(screen.getByRole('checkbox', { name: tr.$tr('shuffleAnswersLabel') })).toBeInTheDocument();
});

Yeah, it'd be great to have a small regression test here.

✅ Resolved — addressed in the current code.

suggestion: newDescriptor.defaultState never exists — both descriptors keep _defaultState as a module-local in parse.js, and grep -rn defaultState finds only this call site. The ternary always takes the parse('', []) branch. Either drop the dead branch or expose defaultState on the descriptors and add it to REQUIRED_KEYS in defineInteraction.js so it's enforced. Moot if the blocking finding removes the conversion path.

✅ Resolved — addressed in the current code.

suggestion: #6033 places the selector in QTIItemEditor's header row — "QTIItemEditor renders a type selector in the card header" — with the portal bridging the plugin's controls up into it. Here it's inverted: QuestionTypeSelector lives inside InteractionSection (i.e. inside question-card-body) and AnswerSettings teleports down into it, so neither control is in the header. The component-ownership argument for this is reasonable and with border-bottom: fineLine it will probably read as a second header band, but the visual result is the part the issue actually constrains — worth confirming against the design before merge. (Manual QA didn't run on this review, so I have no rendered evidence either way.)

✅ Resolved — addressed in the current code.

suggestion: defaultProps doesn't match the component's props. QuestionTypeSelector declares exactly two, questionType and settingsTargetId (both required); this passes questionTypeOptions and mode, neither of which exists, and omits settingsTargetId — so every render logs a missing-required-prop warning and the target div renders without an id. questionTypeOptions here is inert; the real options come from the jest.mock of ../../../interactions. If the blocking finding is taken, questionTypeOptions becomes a real prop and the mock goes away.

Also, it('disables selector when only one option available') asserts is-disabled on document.querySelector('.ui-select') — a KDS-internal class name that will break silently on a KDS upgrade. toBeDisabled() on the combobox role would survive it.

nitpick: neither this component nor ChoiceInteractionEditor (ChoiceInteractionEditor.vue:565) emits update:questionType — the selector moved out to InteractionSection. Both declarations document an event that doesn't exist.

nitpick: the JSDoc above (lines 20-21) still says max-choices defaults to 0 = unlimited and min-choices defaults to 0. Both are now undefined when the attribute is absent — which is the point of this change, and why the two parse.spec.js assertions moved to toBeUndefined(). Worth saying so in the comment, since "absent vs. explicit 0" is now load-bearing for showAnswerCount.

nitpick: InteractionSection generates settingsTargetId internally and declares no targetId prop — this looks like a leftover from an earlier iteration, and lands as a stray DOM attribute.